fix(alfred): read a large decimal subaccount ID as decimal - #8030
Open
yhabib wants to merge 3 commits into
Open
Conversation
The Encode ICRC-1 Account utility read an all-digit subaccount ID with Number(). Above Number.MAX_SAFE_INTEGER it re-read the same digit string as hexadecimal. A user who typed the decimal 18446744073709551615 got the account for 0x18446744073709551615. A deposit then landed on a different ICRC-1 subaccount than the user intended. The utility now converts every all-digit ID with BigInt and writes the value big-endian into the 32-byte subaccount. Small IDs keep the result that SubAccount.fromID gave. An ID that needs more than 32 bytes raises the existing subaccount error. A digit-length guard rejects an ID with more than 78 significant digits before the BigInt conversion. The largest 32-byte value has 78 decimal digits, so a longer string cannot be valid. The guard also stops a long pasted string from blocking the tab in BigInt.
The spec opens the util from the command palette and checks that a decimal subaccount ID above Number.MAX_SAFE_INTEGER encodes as decimal. It also checks the largest subaccount, the error above 32 bytes, and that a 200000 character paste shows the error inside 3 seconds. The spec is test.fixme. No local replica runs on this machine, because dfx 0.32.0 ships pocket-ic-server 13.0.0 and that server rejects the saved snsdemo snapshot state. Remove .fixme when the replica starts again.
|
✅ No security or compliance issues detected. Reviewed everything up to 8fc8370. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current parsing order still treats 64-character all-digit inputs as hex (reintroducing “decimal read as hex” behavior for that case), and should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a security-relevant parsing bug in the Alfred “Encode ICRC-1 Account” utility where large all-digit subaccount IDs could be misinterpreted as hexadecimal, producing the wrong ICRC account text and potentially misdirecting deposits.
Changes:
- Updated subaccount parsing to treat all-digit inputs as decimal via
BigInt, with a digit-length guard to avoid expensive conversions on long pasted strings. - Added component tests covering large decimal values, hex-vs-decimal correctness, overflow, very long input, and leading zeros.
- Added a (currently
test.fixme) Playwright e2e spec documenting end-to-end behavior and the long-paste responsiveness requirement; moved changelog entry under Security.
File summaries
| File | Description |
|---|---|
| frontend/src/lib/components/alfred/BuildIcrcAccountUtil.svelte | Switches decimal parsing to BigInt-based conversion with digit-length guard. |
| frontend/src/tests/lib/components/alfred/BuildIcrcAccountUtil.spec.ts | Adds component tests for the corrected decimal parsing path and guard behavior. |
| frontend/src/tests/e2e/alfred-icrc-account.spec.ts | Adds an e2e (fixme) spec documenting palette reachability and long-input responsiveness. |
| CHANGELOG-Nns-Dapp-unreleased.md | Records the change under the Security section. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Remove the wall-clock duration assertion on the very long paste step. It can flake across CI machines. The functional assertion, that the error shows, still covers the guard. Add a KEY_DEBOUNCE_MS constant for the Escape key delay. This matches the pattern in mobile-alfred.spec.ts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XCBQvbTVxZ26fQiSsxSuGW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
The Alfred "Encode ICRC-1 Account" util read a large decimal subaccount ID with
Number(). AboveNumber.MAX_SAFE_INTEGER, it silently re-read the same digit string as hexadecimal. A user who typed the decimal18446744073709551615got the account for hex0x18446744073709551615, a different subaccount. Funds sent to that address land on the wrong subaccount.Changes
parseDecimalSubAccountto convert every all-digit ID withBigIntand write it big-endian into the 32-byte subaccount.BigIntconversion, so a long pasted string cannot slow the input.test.fixmePlaywright spec that documents the end-to-end check once the local replica works again.Securitysection.Tests
Added tests cover the value above
MAX_SAFE_INTEGER, the "never read as hex" case, a value above 32 bytes, a very long digit string, and leading zeros.npx vitest runpasses: 673 test files, 5910 tests, 9 skipped.npm run checkandnpx eslint --max-warnings 0 .both pass with 0 errors.Todos